@codex review

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 0e85e88b51

Workflow [PR], commit [e1d1b30]

@k-morozov , could you describe a scenario that requires this hardening?
May be we have an issue and/or a stacktrace?
Or a particular item in mounts-and-lifecycle.md?
Hm ... actually it is CasGcScheduler::stop joins the worker threads outside the mutex that guards them (2031-triage CAS-050) {#gc-scheduler-stop-join-race}

@k-morozov , could you describe a scenario that requires this hardening? May be we have an issue and/or a stacktrace? Or a particular item in mounts-and-lifecycle.md? Hm ... actually it is CasGcScheduler::stop joins the worker threads outside the mutex that guards them (2031-triage CAS-050) {#gc-scheduler-stop-join-race}

Not quite. Start and stop have a guard on the level above in current design. Data race related with requestRoundSoon (checking joinable without lock).

Umbrella review: PR #2326 "CAS: gc scheduler stop join race"

Head 74c6f96c492, base antalya-26.6, 4 files (+120/-20). Multi-perspective review (UX/contract, architecture, minimal-diff, code quality, concurrency, lifetime/exception safety, tests, compatibility, deep audit) plus a CI triage.

Summary

The PR closes the reachable data race between requestRoundSoon reading thread.joinable() and stop calling join outside the mutex. After the change requestRoundSoon never touches the thread objects, and thread/hb_thread are only touched under the new threads_mutex by start/stop. That fix is correct by construction.

The PR also bundles a second, separable fix: start now rolls back and rethrows when spawning a worker fails. This repairs a real pre-existing defect: in the old code thread was created before hb_thread, so if the heartbeat spawn threw, the scheduler stayed registered in the storage member with the round loop running and no heartbeat, and a retried SYSTEM CAS GC START silently returned because thread.joinable() was true. (This was a silent half-start, not a process abort.)

No blockers. Verdict: request changes (minor).

Major

1. The targeted race is fixed but never exercised, and TSan is excluded

• No test calls requestRoundSoon concurrently with stop on a bare CasGcScheduler. The existing ConcurrentStopStartFromTwoThreadsStaysConsistent drives gcStart/gcStop, which the storage layer already serializes under lifecycle_mutex + gc_scheduler_mutex, so it cannot reach the racing pair.
• The PR checkboxes exclude TSan and MSan; CI shows those lanes skipped. The backlog item (2031-triage CAS-050) names the defect as a TSan red.
• Fix: add a hammer test with one thread looping requestRoundSoon and another looping stop/start on the same scheduler instance, and run the branch with TSan enabled before merge.

2. round_requested leaks across a failed start

• start sets Running under mutex, releases mutex, then spawns threads. A concurrent requestRoundSoon in that window sets round_requested = true. The catch block resets scheduler_state and i_am_leader but not round_requested. The old start held mutex for its whole body, so this window is new.
• Effect: the next successful start runs an immediate round instead of waiting interval. Benign, but the rollback is incomplete.
• Fix: clear round_requested next to scheduler_state = SchedulerState::Stopped in the catch block.

Minor

3. Stale comments still reference the removed stopping flag — CasGcScheduler.h:166 (waitForTerminalSelfExitForTest doc) and CasGcScheduler.h:239 (*_exited_on_terminal_for_test doc). Rename to scheduler_state flipping to Stopped.

4. Storage-layer comment above snapshot->start() in gcStart is now wrong. It says start "spawns threads but joins nothing, so it does not block". On the rollback path start now joins the worker it managed to spawn while gcStart holds lifecycle_mutex and gc_scheduler_mutex. The startup comment claiming "only the fault-injection hook can throw" is likewise stale now that start has failpoints.

5. StartFailureRollsBackAndCanBeRetried proves less than its name. isQuiescent only reads round_in_flight, which no round ever set, so the assertion is a tautology. The retry only proves rollback because a missed join would abort() the test binary via ThreadFromGlobalPool move-assign. For the fail_before_heartbeat_worker_start iteration no thread exists at all, so the join logic is not exercised. Suggested: add EXPECT_FALSE(sched.gcHealth().is_leader) after the failure, an immediate EXPECT_NO_THROW(sched.stop()), and after the retried start assert something only a running scheduler produces (e.g. requestRoundSoon followed by a wait for a round-log record).

6. StopClearsLeadershipAfterManualRoundWithoutStart passes on the old code too. Old stop cleared i_am_leader unconditionally. Keep the test, but it is not evidence for the race fix.

7. threads_mutex is undocumented and held across join. Every production caller already serializes start/stop via gc_scheduler_mutex, so three reviewers argued it is unnecessary. Recommendation: keep it (it also serializes the destructor and direct unit-test callers, and is cheap), but add a comment stating its role, the lock order threads_mutex → mutex, and that no other method may take it because it is held across the join. Unlike CasMountRuntime::stopBackgroundWorkers, which joins outside its lock, this relies on the mutex guarding nothing else.

8. start doc comment does not mention the new throw/rollback contract. Add: may throw when spawning a worker fails; on failure the scheduler is back to Stopped and start may be called again.

9. Thread creation order silently flipped (hb_thread now before thread). Harmless, but a one-line comment would prevent it being read as accidental.

10. PR description undersells the second fix. The reviewer's question ("what scenario requires this hardening?") is best answered by the half-start scenario in the Summary above. Suggest stating it explicitly in the description.

Needs verification

• Whether the head compiles without -Wthread-safety warnings; the TSA annotations were checked by inspection only. The UniqueLock + TSA_NO_THREAD_SAFETY_ANALYSIS predicate idiom matches BackgroundSchedulePool.cpp, so a warning is unlikely.
• Pre-existing, not from this diff: runOneRoundNow can re-set i_am_leader after stop returned when both are called directly on the class. Production serializes them under gc_scheduler_mutex. Worth one sentence in the header saying the class relies on that.

Suggested split

• Commit 1: race fix — SchedulerState replacing stopping, requestRoundSoon gating on state, threads_mutex with its comment.
• Commit 2: start rollback + failpoints + retry test, with the half-start scenario in the message.
• Follow-ups: TSA annotations / UniqueLock conversion; a running/stopped column in system.cas_mounts so operators can tell "GC stopped" from "running but not leader" (currently both read is_leader = 0).

CI status: root cause of the two red stateless shards

Both Stateless tests (amd_asan_ubsan, cas s3 storage, parallel) shards fail for one root cause, none of it related to this PR. Evidence is from the shard artifacts (server logs, metric_log, query_log, harness log with the lldb dump).

Chain of events (identical on both shards)

1. Global memory tracker snaps to ASan RSS. Eight minutes into each run the global MemoryTracking counter dipped a few MB below zero (MemoryTrackingUncorrected = -6.1 MB) while only ~200 MB was actually tracked. MemoryWorker's non-jemalloc branch then "corrected" the tracker to resident memory, which under ASan is shadow and quarantine: shard 1 jumped from 0.18 GiB to 18.19 GiB at 20:31:23, shard 2 from 0.06 GiB to 17.86 GiB at 20:32:22. The tracker never came down again: it stayed at 18.4 to 19.8 GiB for the remaining 1.5 to 2.5 hours while real RSS varied between 7.9 and 15 GiB. Largest real query peak in the whole window was 1.25 GiB. (src/Common/MemoryWorker.cpp:923 updateAllocated(resident) when total_memory_tracker.get() < 0; this is the mechanism already recorded for PR CAS improvements #2300 run 8, still unfixed in this tree. The plain ASan lanes on sibling PRs never dip below zero and are fully green.)

2. Dynamic hard limit shrinks below the phantom amount. Commit a344d64 (in antalya-26.6) makes MemoryWorker continuously set the total hard limit to (RSS + host available) * 0.9, floored at RSS + 64 MiB. The floor uses RSS, not the tracker amount, so whenever host free memory shrank or the server's own RSS dropped, the limit fell to 13.8 to 17 GiB while the tracker still said 18.5 GiB. Result: server-wide MEMORY_LIMIT_EXCEEDED storms on every tracked allocation of 1 to 4 MiB (shard 1: 19288 errors at 22:06 and 11059 at 22:07; shard 2: bursts at 20:58, 21:28, 22:34 to 22:38 and others). This also rejects new TCP connections (Cannot initialize connection), which the harness sees as Connection reset by peer.

3. Thread counts silently reduced (shard 2's "result differs" group). Upstream commit c1ab338 caps max_threads by free memory = hard limit minus tracker amount, 1 GiB per thread by default; max_insert_threads is additionally capped by that reduced max_threads. With 18.5 GiB of phantom usage, "free" memory was 0.5 to 3.6 GiB, so pipelines got 1 to 3 threads. Verified against three failing tests: 03403 asked for 4 threads at 20:52:16 with 3.59 GiB free and got 3; 02981 asked for 8 insert threads at 22:03:23 with 2.2 GiB free and got 2; 01883 asked for 8 at 22:27:04 with 0.5 GiB free and got 1. The CPU-slot scheduler is not involved (zero delayed slots, upscales, downscales or preemptions all run).

4. Shard 1 only: the storm fenced the CAS mount and hung every DROP. The mount lease renewer reads cas_s3/gc/server-roots/stateless-ca-s3/mount through ReadBufferFromS3, which allocates a 4 MiB tracked buffer. All 11 retries in 10.8 s failed with MEMORY_LIMIT_EXCEEDED, the 30 s lease deadline passed, and at 22:06:43 the renewer logged fenced after 1 physical attempts in 10825 ms (classification=external_lease_deadline). The remount loop's pool_identity_probe reads _pool_meta the same way, failed identically, and backed off (attempts 2 to 7, 1 s to 16 s). With the mount fenced, part removal throws NETWORK_ERROR ("mount lease not held"), DatabaseCatalog logged Cannot drop table ... Will retry later 125 times, and every DROP DATABASE blocked in waitTableFinallyDropped (six handler threads in the lldb dump). The per-test cleanup of clickhouse-test therefore hung, the 799 s timeout fired, and the harness declared Server died and sent SIGTERM. The same sequence happened once earlier at 20:51 (fenced after 1 physical attempts in 9224 ms) and recovered after the 36.5 s token-observation window; at 22:06 the storm lasted longer than the backoff.

Attribution per failure

Nothing references CasGcScheduler, GC start/stop or requestRoundSoon; unit tests are green with both new tests passing.

Fixes that follow from the evidence

• MemoryWorker non-jemalloc branch: on a negative tracker amount reset to 0 (or log and keep) instead of snapping to ASan RSS. This removes steps 2 to 4 entirely on sanitizer lanes.
• Dynamic hard limit: floor at max(RSS, tracker amount) + margin, otherwise the limit can sit below what the tracker already accounts for and every allocation fails.
• CAS lease renewal and pool-identity probe: these control-plane reads must not depend on a 4 MiB tracked buffer; read with a small buffer or under MemoryTrackerBlockerInThread, so memory pressure degrades queries, not mount liveness. Losing the mount to a transient allocation failure is a fail-closed cascade that turns every DROP into an infinite wait.
• Open sub-question: why only the CAS lane dips below zero (plain ASan lanes never do). Discriminator: watch MemoryTrackingUncorrected on a local ASan run with --cas-s3-storage and bisect which allocation path frees more than it tracked.

Regression suites

cas_selects: the known port-exhaustion cause (compact parts plus column-subset reads reset every S3 connection). cas_alter_attach_3 and cas_s3_cache_lightweight_delete_4 fail the same scenarios on merged base PR #2327; their root cause was not established in this review and needs its own investigation.

Verdict

Request changes (minor). Minimum actions:

• Clear round_requested in the start rollback path.
• Fix the two stale stopping comments and the "does not block" comment at the gcStart call site.
• Comment threads_mutex; document the start throw/retry contract.
• Add the concurrent requestRoundSoon vs stop test and run the branch once with TSan enabled.
• Update the PR description with the half-start scenario.